Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bin/brew: handle missing $HOME. #15818

Merged
merged 1 commit into from
Aug 4, 2023
Merged

bin/brew: handle missing $HOME. #15818

merged 1 commit into from
Aug 4, 2023

Conversation

MikeMcQuaid
Copy link
Member

Try to build it using $USER or $LOGNAME and, if both are missing, just give up.

See #15787 (comment)

Try to build it using `$USER` or `$LOGNAME` and, if both are missing,
just give up.
@MikeMcQuaid MikeMcQuaid added the critical Critical change which should be shipped as soon as possible. label Aug 4, 2023
@MikeMcQuaid MikeMcQuaid merged commit ef4731d into Homebrew:master Aug 4, 2023
@MikeMcQuaid MikeMcQuaid deleted the brew_env_home branch August 4, 2023 08:21
@cdalvaro
Copy link
Contributor

cdalvaro commented Aug 4, 2023

I have a comment.

Although this is not the common case, the $USER and the HOME directory can be different (as it is my case):

echo "$USER"
carlos.alvaro

pwd
/Users/Carlos

However, this way for getting the HOME directory seems to work on Linux/BSD/macOS operative systems:

eval echo -n "~$USER"
/Users/Carlos

@MikeMcQuaid
Copy link
Member Author

@cdalvaro Not interested in making it more involved than it currently is or using eval. If it's detecting things wrongly: the solution is to set $HOME correctly.

@MikeMcQuaid
Copy link
Member Author

@cdalvaro However, literally the only thing it's being used for in this file is for these user-specific environment files.

@cdalvaro
Copy link
Contributor

cdalvaro commented Aug 4, 2023

I'm sorry.

After this change, I've checked if the salt issue has been solved. But it stills happening with the new error:

[salt.state       :325 ][ERROR   ][9799] An error was encountered while removing package(s): Path not found: Error: $HOME or $USER or $LOGNAME must be set to run brew./bin/brew
[salt.loaded.int.module.cmdmod:920 ][ERROR   ][13361] Command 'brew' failed with return code: 1

Finally, I've found the issue. salt is trying to find the Homebrew's prefix by calling brew --prefix using the root user (since my salt-minion is started via a Launch Daemon)

def _homebrew_bin():
    """
    Returns the full path to the homebrew binary in the PATH
    """
    ret = __salt__["cmd.run"]("brew --prefix", output_loglevel="trace")
    ret += "/bin/brew"
    return ret

Apparently, the environment in this situation is very limited:

LC_PAPER=C
LC_ADDRESS=C
LC_MONETARY=C
LC_NUMERIC=C
LC_TELEPHONE=C
PATH=/opt/homebrew/bin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
LC_MESSAGES=C
LC_IDENTIFICATION=C
LC_COLLATE=C
PWD=/private/var/root
LC_MEASUREMENT=C
XPC_FLAGS=0x0
XPC_SERVICE_NAME=0
SHLVL=1
LANGUAGE=C
HOMEBREW_PREFIX=/opt/homebrew
LC_CTYPE=C
LC_TIME=C
LC_NAME=C
_=/usr/bin/env

(I have added PATH and HOMEBREW_PREFIX manually to my daemon's plist file)

So as you can see, no HOME neither USER are present in the environment.

I'm preparing a fix for salt in order to manage this change in the brew command. Something like this:

def _homebrew_bin():
    """
    Returns the full path to the homebrew binary in the PATH
    """
    bin_brew = "/bin/brew"

    try:
        ret = __salt__["cmd.run"]("brew --prefix", output_loglevel="trace", raise_err=True)
    except CommandExecutionError:
        log.warn("Unable to find homebrew prefix by running 'brew --prefix'. Trying with HOMEBREW_PREFIX env variable.")
        ret = os.getenv("HOMEBREW_PREFIX")

    if ret is not None:
        log.debug("Found homebrew prefix: {}".format(ret))
        return ret + bin_brew

    ret = "/opt/homebrew" if __grains__["cpuarch"] == "arm64" else "/usr/local"
    ret += bin_brew

    if not os.path.exists(ret):
        log.error("Command {} does not exist. Unable to find brew command".format(ret))
        raise CommandExecutionError("Failed to get guess homebrew prefix. Please, set HOMEBREW_PREFIX env variable", info={"result": {}})

    return ret

Please, let me know if you consider there is a better approach for this situation.

@Bo98
Copy link
Member

Bo98 commented Aug 4, 2023

Ignoring this user config code completely for a moment, how has a missing HOME ever worked for:

if [[ -n "${HOMEBREW_MACOS}" ]]
then
HOMEBREW_DEFAULT_CACHE="${HOME}/Library/Caches/Homebrew"
HOMEBREW_DEFAULT_LOGS="${HOME}/Library/Logs/Homebrew"
HOMEBREW_DEFAULT_TEMP="/private/tmp"
else
CACHE_HOME="${XDG_CACHE_HOME:-${HOME}/.cache}"
HOMEBREW_DEFAULT_CACHE="${CACHE_HOME}/Homebrew"
HOMEBREW_DEFAULT_LOGS="${CACHE_HOME}/Homebrew/Logs"
HOMEBREW_DEFAULT_TEMP="/tmp"
fi

?

If you can skip the user config stuff and see how the above code works for you, that would be useful information.

@cdalvaro
Copy link
Contributor

cdalvaro commented Aug 5, 2023

Salt only runs brew as root to get the Homebrew's prefix.

Once it has find the brew command, all other commands are executed by logging in with the owning brew user.

This is how Salt runs brew commands (I explained it initially here: #15787 (comment)):

def _call_brew(*cmd, failhard=True):
    """
    Calls the brew command with the user account of brew
    """
    user = __salt__["file.get_user"](_homebrew_bin())
    runas = user if user != __opts__["user"] else None
    _cmd = []
    if runas:
        _cmd = ["sudo -i -n -H -u {} -- ".format(runas)]
    _cmd = _cmd + [salt.utils.path.which("brew")] + list(cmd)
    _cmd = " ".join(_cmd)


    runas = None
    result = __salt__["cmd.run_all"](
        cmd=_cmd,
        runas=runas,
        output_loglevel="trace",
        python_shell=False,
    )
    if failhard and result["retcode"] != 0:
        raise CommandExecutionError("Brew command failed", info={"result": result})
    return result

(https://github.com/saltstack/salt/blob/4e8b77df671fd756970fe4fb08122fba9b47c50b/salt/modules/mac_brew_pkg.py#L105-L126)

It tries to figure out brew's owner and then runs the command as this user (sudo -H sets the HOME)

@MikeMcQuaid
Copy link
Member Author

If you can skip the user config stuff and see how the above code works for you, that would be useful information.

It's likely it just doesn't and puts these files into the wrong place.

@github-actions github-actions bot added the outdated PR was locked due to age label Sep 5, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants